feat(setupChecks): Enhance output for SecurityHeaders #55641
Conversation
Improve the output when headers are missing/incorrect to make troubleshooting easier. Minor code reformatting for readability. New class and main function docblocks. Signed-off-by: Josh <[email protected]>
joshtrichards
requested review from
ArtificialOwl and
salmart-dev
and removed request for
a team
Signed-off-by: Josh <[email protected]>
| * on the Nextcloud instance. The check issues warnings or informational messages if recommended | ||
| * security headers are missing, malformed, or set to unsafe values. | ||
| * | ||
| * This class is used by the Nextcloud setup process to ensure that the web server delivers |
There was a problem hiding this comment.
| * This class is used by the Nextcloud setup process to ensure that the web server delivers | |
| * This class is used to ensure that the web server delivers |
It’s not part of setup (as in installation), it’s in the admin overview.
| * Class SecurityHeaders | ||
| * |
There was a problem hiding this comment.
| * Class SecurityHeaders | |
| * |
I would strip these lines as they bring no information.
| * Executes the security header setup check. | ||
| * |
There was a problem hiding this comment.
| * Executes the security header setup check. | |
| * |
| * | ||
| * @return SetupResult Result of the security headers setup check. |
There was a problem hiding this comment.
| * | |
| * @return SetupResult Result of the security headers setup check. |
| '- The `%1$s` HTTP header is not set to `%2$s`. Some features ' | ||
| . 'might not work correctly, as it is recommended to adjust this ' | ||
| . 'setting accordingly.', |
There was a problem hiding this comment.
I’m not a big fan of the string split, it breaks searching for the error string in the code. What’s the added value?
| . 'If you believe this is incorrect, review your `overwrite.cli.url` and `trusted_domains` settings. ' | ||
| . 'These settings may include URLs that do not use HTTPS or bypass your reverse proxy, ' | ||
| . 'which can affect header checks. ' | ||
| . 'Additionally, ensure your DNS records and server configuration are consistent with your HTTPS setup.', |
There was a problem hiding this comment.
We should definitely not include unstranslated text in setupchecks results.
I’m also unsure about this added warning text, ideally we should improve the checks to make them less error-prone instead, and for special case document that in the linked documentation.
Maybe we should show a general setting on the overview page on which URL is used by the server for loopback checks, and make it configurable for special cases. Not sure.
2 participants
Summary
Improve the output when headers are missing/incorrect to make troubleshooting easier.
Minor code reformatting for readability.
New class and main function docblocks.
TODO
Checklist
3. to review, feature component)stable32)